ROSAENG-130 | feat: add support for AWS podidentity#67
ROSAENG-130 | feat: add support for AWS podidentity#67cdoan1 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
Conversation
Add support for using AWS Pod Identity with Secrets Store CSI driver to mount database credentials from AWS Secrets Manager, eliminating the need for Kubernetes Secrets. This includes a new SecretProviderClass template, file-based credential flags for the application, and comprehensive Helm chart configuration options. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Move AWS secretName to aws subsection and clean up adapter configuration comments for better clarity. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @cdoan1. Thanks for your PR. I'm waiting for a openshift-hyperfleet member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughThis pull request introduces AWS Secrets Manager integration with Kubernetes pod identity support for external database credential management. The changes add conditional deployment configuration to use CSI Secrets Store volumes when pod identity is enabled, a new SecretProviderClass template for AWS Secrets Manager integration, and corresponding values file entries to control this behavior. When pod identity is disabled, the deployment continues using traditional Kubernetes Secrets. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes introduce a new pod identity feature across multiple template and configuration files with conditional branching logic. Review requires understanding the interaction between CSI Secrets Store, pod identity mechanisms, and deployment configuration, along with verifying proper conditional rendering of templates based on multiple configuration flags. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/templates/deployment.yaml`:
- Around line 43-50: The --db-sslmode flag is only rendered inside the
pod-identity branch so .Values.database.external.sslMode is ignored when using
external DB with secret mount mode; update charts/templates/deployment.yaml to
ensure the --db-sslmode={{ .Values.database.external.sslMode | default "require"
}} argument is emitted whenever .Values.database.external.enabled is true
(either move it outside the usePodIdentity conditional or add a separate
conditional for external enabled and not usePodIdentity), referencing the
existing conditional keys .Values.database.external.enabled and
.Values.database.external.usePodIdentity and the flag name --db-sslmode to
locate where to add/move the line.
- Around line 145-151: The CSI volumeAttributes currently always renders a
secretProviderClass even when
.Values.database.external.createSecretProviderClass is false and
.Values.database.external.secretProviderClass is empty; update the template
around the csi block to conditionally render the secretProviderClass key only
when the computed value is non-empty (check
.Values.database.external.createSecretProviderClass OR
.Values.database.external.secretProviderClass) so the CSI config is omitted if
no provider class is provided; locate the ternary expression that builds the
name (uses include "hyperfleet-api.fullname",
.Values.database.external.secretProviderClass,
.Values.database.external.createSecretProviderClass) and wrap or gate the
secretProviderClass output with an if that ensures the rendered string is not
empty.
In `@charts/templates/secretproviderclass.yaml`:
- Around line 13-16: The SecretProviderClass template currently renders
potentially empty values for region and objectName which defers failure to
runtime; update the template to validate and fail fast by wrapping
.Values.database.external.aws.region and
.Values.database.external.aws.secretName with the Helm required function (e.g.,
required "aws.region is required" .Values.database.external.aws.region) so
template rendering fails when those inputs are empty; ensure the required checks
are applied where region and objectName are inserted in the SecretProviderClass
objects block to prevent creating a pod-mount-time error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a3e0604-0fcb-4da0-b5bf-1c29e58c6e3f
📒 Files selected for processing (3)
charts/templates/deployment.yamlcharts/templates/secretproviderclass.yamlcharts/values.yaml
| {{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity }} | ||
| csi: | ||
| driver: secrets-store.csi.k8s.io | ||
| readOnly: true | ||
| volumeAttributes: | ||
| secretProviderClass: {{ ternary (printf "%s-db-secrets" (include "hyperfleet-api.fullname" .)) .Values.database.external.secretProviderClass .Values.database.external.createSecretProviderClass | quote }} | ||
| {{- else }} |
There was a problem hiding this comment.
Add a render-time guard for empty secretProviderClass.
At Line 150, if createSecretProviderClass=false and database.external.secretProviderClass is empty, the pod renders with an invalid CSI config and fails at startup.
🔧 Proposed fix
- name: secrets
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity }}
+ {{- $spcName := ternary (printf "%s-db-secrets" (include "hyperfleet-api.fullname" .)) .Values.database.external.secretProviderClass .Values.database.external.createSecretProviderClass }}
+ {{- if empty $spcName }}
+ {{- fail "database.external.secretProviderClass is required when usePodIdentity=true and createSecretProviderClass=false" }}
+ {{- end }}
csi:
driver: secrets-store.csi.k8s.io
readOnly: true
volumeAttributes:
- secretProviderClass: {{ ternary (printf "%s-db-secrets" (include "hyperfleet-api.fullname" .)) .Values.database.external.secretProviderClass .Values.database.external.createSecretProviderClass | quote }}
+ secretProviderClass: {{ $spcName | quote }}
{{- else }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/templates/deployment.yaml` around lines 145 - 151, The CSI
volumeAttributes currently always renders a secretProviderClass even when
.Values.database.external.createSecretProviderClass is false and
.Values.database.external.secretProviderClass is empty; update the template
around the csi block to conditionally render the secretProviderClass key only
when the computed value is non-empty (check
.Values.database.external.createSecretProviderClass OR
.Values.database.external.secretProviderClass) so the CSI config is omitted if
no provider class is provided; locate the ternary expression that builds the
name (uses include "hyperfleet-api.fullname",
.Values.database.external.secretProviderClass,
.Values.database.external.createSecretProviderClass) and wrap or gate the
secretProviderClass output with an if that ensures the rendered string is not
empty.
There was a problem hiding this comment.
@cdoan1, sounds good! The proposed fix above adds the $spcName variable and the {{- if empty $spcName }} guard to fail early at render time, so it should be straightforward to drop in. Feel free to ping me once it's in and I'll take another look. 👍
🧠 Learnings used
Learnt from: rafabene
Repo: openshift-hyperfleet/hyperfleet-api PR: 43
File: charts/templates/servicemonitor.yaml:1-1
Timestamp: 2026-02-02T18:38:24.725Z
Learning: In the openshift-hyperfleet/hyperfleet-api repo, Helm template files under charts/templates/ use Go template syntax (e.g., {{ .Values.xxx }}). Do not lint or flag yamllint errors for these files, as standard YAML linters do not understand Helm Go templates. If you must lint, configure yamllint to skip these files or use a Helm-aware linter.
| region: {{ .Values.database.external.aws.region | quote }} | ||
| objects: | | ||
| - objectName: {{ .Values.database.external.aws.secretName | quote }} | ||
| objectType: "secretsmanager" |
There was a problem hiding this comment.
Fail fast when AWS SecretProviderClass inputs are empty.
At Line 13 and Line 15, empty region or objectName can render successfully but fail later at pod mount/runtime. Guard these with required so misconfiguration is caught during template rendering.
🔧 Proposed fix
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }}
+{{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }}
+{{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }}
apiVersion: secrets-store.csi.x-k8s.io/v1
kind: SecretProviderClass
metadata:
@@
spec:
provider: aws
parameters:
usePodIdentity: "true"
- region: {{ .Values.database.external.aws.region | quote }}
+ region: {{ $awsRegion | quote }}
objects: |
- - objectName: {{ .Values.database.external.aws.secretName | quote }}
+ - objectName: {{ $awsSecretName | quote }}
objectType: "secretsmanager"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| region: {{ .Values.database.external.aws.region | quote }} | |
| objects: | | |
| - objectName: {{ .Values.database.external.aws.secretName | quote }} | |
| objectType: "secretsmanager" | |
| {{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }} | |
| {{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }} | |
| {{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }} | |
| apiVersion: secrets-store.csi.x-k8s.io/v1 | |
| kind: SecretProviderClass | |
| metadata: | |
| name: {{ .Values.database.external.secretProviderClassName }} | |
| spec: | |
| provider: aws | |
| parameters: | |
| usePodIdentity: "true" | |
| region: {{ $awsRegion | quote }} | |
| objects: | | |
| - objectName: {{ $awsSecretName | quote }} | |
| objectType: "secretsmanager" | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/templates/secretproviderclass.yaml` around lines 13 - 16, The
SecretProviderClass template currently renders potentially empty values for
region and objectName which defers failure to runtime; update the template to
validate and fail fast by wrapping .Values.database.external.aws.region and
.Values.database.external.aws.secretName with the Helm required function (e.g.,
required "aws.region is required" .Values.database.external.aws.region) so
template rendering fails when those inputs are empty; ensure the required checks
are applied where region and objectName are inserted in the SecretProviderClass
objects block to prevent creating a pod-mount-time error.
There was a problem hiding this comment.
@cdoan1, sounds good! Here's a quick recap of the proposed change for reference when you implement it:
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }}
{{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }}
{{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }}
apiVersion: secrets-store.csi.x-k8s.io/v1
kind: SecretProviderClass
metadata:
name: {{ include "hyperfleet-api.fullname" . }}-db-secrets
namespace: {{ .Release.Namespace }}
labels:
{{- include "hyperfleet-api.labels" . | nindent 4 }}
spec:
provider: aws
parameters:
usePodIdentity: "true"
region: {{ $awsRegion | quote }}
objects: |
- objectName: {{ $awsSecretName | quote }}
objectType: "secretsmanager"Using required ensures helm template / helm install fails immediately with a clear error message if either value is missing, rather than silently producing a broken SecretProviderClass that only fails at pod mount time.
🧠 Learnings used
Learnt from: rafabene
Repo: openshift-hyperfleet/hyperfleet-api PR: 43
File: charts/templates/servicemonitor.yaml:1-1
Timestamp: 2026-02-02T18:38:24.725Z
Learning: In the openshift-hyperfleet/hyperfleet-api repo, Helm template files under charts/templates/ use Go template syntax (e.g., {{ .Values.xxx }}). Do not lint or flag yamllint errors for these files, as standard YAML linters do not understand Helm Go templates. If you must lint, configure yamllint to skip these files or use a Helm-aware linter.
|
/ok-to-test |
2 participants
On AWS, the prod deployment can use AWS MQ and AWS RDS
With eks clusters, we can generate the secrets in aws secret manager, and sync these secrets down to the pod via pod identity and secretprovderclass.
Summary by CodeRabbit